Repository navigation
Conversation
nwt
left a comment
nwt
left a comment
There was a problem hiding this comment.
Only non-nit is the comment on db/pools/config.go.
| ) | ||
|
|
||
| var VecBatchSize uint32 = 10 * 1024 | ||
| var DefaultInputCap uint32 = 10 * 1024 |
There was a problem hiding this comment.
Nit: No reason for this to be a variable.
| var DefaultInputCap uint32 = 10 * 1024 | |
| const defaultInputCap uint32 = 10 * 1024 |
| @@ -0,0 +1,9 @@ | |||
| script: | | |||
| seq 100 | super -inputcap 5 -framecap 5 -o out.bsup - | |||
There was a problem hiding this comment.
Nit: Put a prefix on this flag since it only affects BSUP.
| seq 100 | super -inputcap 5 -framecap 5 -o out.bsup - | |
| seq 100 | super -inputcap 5 -bsup.framecap 5 -o out.bsup - |
There was a problem hiding this comment.
Let's discuss. I'm gonna leave this for now and we can visit flags names overall.
| "tab size to pretty print JSON and Super JSON output (0 for newline-delimited output") | ||
| fs.StringVar(&f.outputFile, "o", "", "write data to output file") | ||
| fs.BoolVar(&f.BSUP.Rows, "rows", false, "output BSUP in row format instead of columns") | ||
| fs.Uint64Var(&f.BSUP.FrameCap, "framecap", 10000, "number of values per BSUP frame") |
There was a problem hiding this comment.
Nit: Put this after "color" so these remain mostly ordered by flag name. Even better, call it "bsup.framecap" since it applies only to BSUP and then put it before "color".
| ObjectCap uint64 `json:"objectcap"` | ||
| FrameCap uint64 `json:"framecap"` |
There was a problem hiding this comment.
Nit: I don't feel strongly about this but knobs like these are usually suffixed with "lim", "limit", or "max" so one of those instead of "cap" might make their behavior a little clearer to system users and code readers. (My initial reaction was, "'Cap' probably means maximum here but if that were the case it'd just be 'max" so maybe it means something else.")
There was a problem hiding this comment.
Second nit:
| ObjectCap uint64 `json:"objectcap"` | |
| FrameCap uint64 `json:"framecap"` | |
| ObjectCap uint64 `json:"object_cap"` | |
| FrameCap uint64 `json:"frame_cap"` |
There was a problem hiding this comment.
I'm gonna leave these as is since we will discuss and replace them all in a subsequent PR. No since replacing all the tests now and changing again.
| func NewColumnWriter(w io.WriteCloser) *ColumnWriter { | ||
| return NewColumnWriterWithCap(w, DefaultFrameCap) | ||
| } | ||
|
|
||
| func NewColumnWriterWithCap(w io.WriteCloser, frameCap uint64) *ColumnWriter { |
There was a problem hiding this comment.
Nit: I think it's tidier to have just one constructor for which the zero value for a parameter gets you a reasonable default.
| fs.BoolVar(&f.Dynamic, "dynamic", false, "disable static type checking of inputs") | ||
| fs.StringVar(&opts.Format, "i", "auto", "format of input data [auto,arrows,bsup,csv,json,line,parquet,sup,tsv,zeek]") | ||
| fs.BoolVar(&f.Static, "static", false, "force static type checking of inputs") | ||
| fs.IntVar(&opts.InputCap, "inputcap", 0, "limit size of batched units of input") |
There was a problem hiding this comment.
Nit: Put this after "i" so these remain ordered by flag name.
| const ( | ||
| DefaultThreshold = 500 * 1024 * 1024 | ||
| DefaultObjectCap = 1 * 1024 * 1024 | ||
| ) |
There was a problem hiding this comment.
Nit:
const DefaultObjectCap = 024 * 1024| if objectCap == 0 { | ||
| objectCap = data.DefaultObjectCap | ||
| } | ||
| if objectCap == 0 { |
There was a problem hiding this comment.
| if objectCap == 0 { | |
| if frameCap == 0 { |
This commit restores the old behavior of database ingest where input is broken up into objects based on configured caps. The old method used bytes threshold but this isn't so easy to implement so we went with number-of-values caps. Bytes limits can be revisited later. We introduced three new flags: -objectcap, -framecap, and -inputcap to control the size of storage objects, the size of BSUP frames, and the size of vector batches on the input. The limits don't break vectors into pieces so the sizes get rounded up to the first vector that exceeds the cap. We didn't want to muck around with breaking vectors up into smaller pieces on these boundaries but we can revisit this later.
2 participants
This commit restores the old behavior of database ingest where input is broken up into objects based on configured caps. The old method used bytes threshold but this isn't so easy to implement so we went with number-of-values caps. Bytes limits can be revisited later.
We introduced three new flags: -objectcap, -framecap, and -inputcap to control the size of storage objects, the size of BSUP frames, and the size of vector batches on the input. The limits don't break vectors into pieces so the sizes get rounded up to the first vector that exceeds the cap. We didn't want to muck around with breaking vectors up into smaller pieces on these boundaries but we can revisit this later.